Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mh/more info feedback component testing #2810

Merged
merged 4 commits into from
Feb 23, 2024

Conversation

Mike-Heneghan
Copy link
Contributor

What:

  • Add tests for the MoreInfoFeedback component
  • Update FeedbackForm to add a data-testid
  • Add a default aria-label for inputs which don't have an explicit input label to meet accessibility requirements

Why:

  • Increasing the test coverage

@Mike-Heneghan Mike-Heneghan self-assigned this Feb 23, 2024
@Mike-Heneghan Mike-Heneghan requested a review from a team February 23, 2024 14:01
Copy link

github-actions bot commented Feb 23, 2024

Removed vultr server and associated DNS entries

Copy link
Member

@jessicamcinchak jessicamcinchak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look good here, thanks for picking this up!

One comment about our error handling pattern on feedback input, which I'm happy to be picked up separately.

});

expect(getByTestId("userCommentTextarea")).toHaveAttribute("required");
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test highlights something interesting - while we can't test that the browser stops form submit in Jest, we'd normally test that an input error message becomes visible when trying to proceed without filling out required fields - but it doesn't seem like we've accounted for that with any of these components and are relying on default browser behavior?

I'd expect us to the use the same GOV.UK pattern of red left border and message that we use for all other public-facing inputs - is there a reason we aren't? I think it's very likely this will be picked up in a11y testing as inconsistent and insufficient.

Screenshot from 2024-02-23 15-43-21

Screenshot from 2024-02-23 15-36-07

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the time this feature was being built I think there were conversations about using the native required although to be fair that might have been scoped to the editor rather than the public view.

I did have a version with the gov uk style errors but they could never actually be seen by users as the required attribute stopped form submission which would be run to show these validation messages so that state could never be reached.

As this was the case at the time I thought I'd reduce complexity but not showing any code which I thought a user could never see.

It'll be really interesting to see what the a11y testing comes back with because on one hand not using required for a required field seems somewhat off? Although as you say it's inconsistent 🤔

@Mike-Heneghan Mike-Heneghan merged commit 733054a into main Feb 23, 2024
12 checks passed
@DafyddLlyr DafyddLlyr deleted the mh/more-info-feedback-component-testing branch February 26, 2024 14:02
@Mike-Heneghan
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants